Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Volume matching #406

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Volume matching #406

wants to merge 1 commit into from

Conversation

lilione
Copy link
Collaborator

@lilione lilione commented Dec 4, 2019

Implement the volume matching algorithm and test cases for it.

@sanket1729
Copy link
Collaborator

We can merge fixed point first. Then we can get this in

@sanket1729
Copy link
Collaborator

Would require a rebase now

@lilione lilione closed this Dec 8, 2019
@lilione lilione reopened this Dec 8, 2019
Copy link
Contributor

@amiller amiller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I can review it further, this is missing any comments or documentation explaining what the code does

return buys, sells


async def volume_matching(ctx, bids):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is missing comments explaining what this function interface is. What is "bids" supposed to contain? What is the type of it? Are there preconditions? What function does this compute?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible try to follow the documentation format guidelines in our contributing guidelines:
https://github.com/initc3/HoneyBadgerMPC/blob/dev/docs/development/contributing.rst#documentation-conventions

@codecov
Copy link

codecov bot commented Dec 8, 2019

Codecov Report

Merging #406 into dev will decrease coverage by 0.23814%.
The diff coverage is 66.66667%.

@@                 Coverage Diff                 @@
##                 dev        #406         +/-   ##
===================================================
- Coverage   77.02727%   76.78913%   -0.23815%     
===================================================
  Files             50          51          +1     
  Lines           5611        5743        +132     
  Branches         859         876         +17     
===================================================
+ Hits            4322        4410         +88     
- Misses          1113        1157         +44     
  Partials         176         176                 

@lilione lilione requested a review from amiller December 17, 2019 08:37
@amiller amiller requested a review from sbellem February 26, 2020 23:26
@amiller amiller removed their request for review February 26, 2020 23:26
@amiller
Copy link
Contributor

amiller commented Feb 26, 2020

We should try to use this PR as an example of the documentation style we want to see in contributed apps

@sbellem
Copy link
Collaborator

sbellem commented Mar 4, 2020

@lilione when you have a chance, could you rebase the PR so that CircleCI will be used to generate the docs and we can then preview them ...

@sbellem
Copy link
Collaborator

sbellem commented Mar 25, 2020

@yunqi I added some commits so that you can build and view the docs locally. To build and view the docs in your browser you can run:

$ make servedocs

To build the docs:

$ make docs

You then need to refresh the browser.

To view the auctions app docs go to http://localhost:58888/apps/auctions.html.

Let me know once you are setup!

@lilione lilione force-pushed the volume-matching branch from 15813d9 to 0f9aca8 Compare May 15, 2020 03:24

Parameters
----------
balances : {address -> {cointype -> balance}}
Copy link
Collaborator

@sbellem sbellem Jun 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the type, I think the convention is usually to specify the type, such as list, dict, str, etc. From the numpy docstring docs:

For the parameter types, be as precise as possible. Below are a few examples of parameters and their types.

Parameters
----------
filename : str
copy : bool
dtype : data-type
iterable : iterable object
shape : int or tuple of int
files : list of str

So in this case that would be dict of dict.

In the description, details about the keys and values can be added, such as keys are addresses, and values are dictionaries in which the keys are coin types and the values are balances ...

Just as a rough example.

@sbellem
Copy link
Collaborator

sbellem commented Jun 29, 2020

This is good to merge. The app works (python apps/auctions/volume-matching.py) and the documentation follows the numpy docstring style.

/cc @amiller

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants